Skip to content

FileDownloadDelegate: pass response header through #680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

FileDownloadDelegate: pass response header through #680

wants to merge 3 commits into from

Conversation

MaxDesiatov
Copy link
Member

Currently FileDownloadDelegate doesn't pass the response header through in its Response associated type, which is satisfied by the Progress inner type declaration. This makes it hard to use in the Swift Concurrency context, since it's no trivial to pass response header around from reportHead closure. Let's fix that by adding a new field that passes the response header in the delegate's response.

This change is not source-breaking and is purely additive, since no existing properties were changed.

Currently `FileDownloadDelegate` doesn't pass the response header through in its `Response` associated type, which is satisfied by the `Progress` inner type declaration. This makes it hard to use in the Swift Concurrency context, since it's no trivial to pass response header around from `reportHead` closure. Let's fix that by adding a new field that passes the response header in the delegate's response.

This change is additive and is not source-breaking.
@dnadoba
Copy link
Collaborator

dnadoba commented Apr 13, 2023

Closing this in favour of #681

@dnadoba dnadoba closed this Apr 13, 2023
@gregcotten
Copy link
Contributor

gregcotten commented Jul 19, 2024

I feel like maybe this shouldn't have been closed. It's not super ergonomic to have to capture the response head via the reportHead closure.

var head: HTTPResponseHead?
let delegate = try FileDownloadDelegate(path: "/some/tmp/file.tmp",
                                        pool: nil,
                                        reportHead: {
        head = $1
        if $1.status.code != 200 {
            $0.cancel()
        }
    },
                                        reportProgress: nil
)

let result = try await HTTPClient.shared.execute(request: request, delegate: delegate).get()

if let head {
   // do something with head
}

@Lukasa
Copy link
Collaborator

Lukasa commented Jul 22, 2024

The fix in 681 was suitable for the use-case in question, where the head wasn't wanted after the download, but so that it could be analysed before the download progressed. If you need the head afterward then we may want to approach this other design instead.

@gregcotten
Copy link
Contributor

In my case I want to inspect the "content-disposition" field to determine a suitable user-facing file name.

@Lukasa
Copy link
Collaborator

Lukasa commented Jul 22, 2024

Ok, so I think it's worth us adding this flow. @dnadoba any objections on your end?

@gregcotten
Copy link
Contributor

I'm happy to make a PR with the change. I don't think the Progress struct should be modified as addressed in this PR since there is a reportHead closure now. We should probably just make a Response struct (instead of the current typealias) with progress + head info, right?

@dnadoba
Copy link
Collaborator

dnadoba commented Jul 23, 2024

We cannot replace the FileDownloadDelegate.Response type alias with a new struct without breaking source.
We can therefore only add the head as new field to the FileDownloadDelegate.Progress struct as proposed in this PR.

I'm fine if we add it. However, if we want to add it we should make the head non-optional and require it as opposed to what is proposed in this PR. This can be done safely because we only report progress (or finish the request and return FileDownloadDelegate.Response which is a type alias to FileDownloadDelegate.Progress) after we have received the head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants